Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Cliffords to QuantumCircuits as Operations #7966

Merged
merged 41 commits into from
Aug 10, 2022

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Apr 20, 2022

Summary

In this PR, we are adding a Clifford object to a QuantumCircuit as an Operation, without first explicitly converting this Clifford into a Gate.

An important point is that a Clifford is no longer decomposed as soon as it is added to a QuantumCircuit. The procedure decompose_clifford only runs when the definition of a Clifford is required, which might only be during a later transpiler pass.

To illustrate an additional benefit of adding Cliffords natively, there is a preliminary transpiler pass OptimizeCliffords that uses the ability to compose Cliffords.

In the near future, other objects will be added as Operations as well.

Details and comments

A Clifford object does not inherit from Instruction. In order to work with Cliffords natively, it must support the following:

  • broadcast_arguments: this is used for expanding (aka broadcasting) arguments. Note that there is an additional PR to improve the broadcasting code.
  • condition: as far as I understand, this is used for conditional if instructions. I don't think it applies to Cliffords, but is currently required by circuit_to_dag. I believe this is something that we can later clean up.
  • _directive: This is a attribute to treat like barrier for transpiler, unroller, drawer. It is currently required by DagCircuit. Again I think that this may be something that we can later clean up.
  • definition: This is the function that builds the definition of the Clifford in terms of more basic gates (i.e. the function that essentially calls decompose_clifford).

This PR essentially restores #7087, but with a few differences:

  • Instruction now also inherits from Operation. As @jakelishman commented during the Qiskit Circuits and Transpiler meeting, this is actually the correct thing to do: instructions are something that we add to quantum circuits, even if we later choose to deprecate Instruction altogether (and moreover this would greatly simplify the deprecation process).
  • The classes CNOTDihedral and Pauli do not yet inherit from Operation. These (and some other classes) will be added to Operation in a later PR (and some experiments are needed to make sure that inheriting Pauli from Operation does not lead to worse performance).
  • QuantumCircuit itself also does not yet inherit from Operation (and possibly some more thought is required whether it eventually should and what would be the additional benefit of this -- as we can already append one quantum circuit to another).

In order to turn condition into a method of Operation, we needed to add the property condition to Instruction.

@alexanderivrii alexanderivrii requested review from a team and ikkoham as code owners April 20, 2022 12:29
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@coveralls
Copy link

coveralls commented Apr 20, 2022

Pull Request Test Coverage Report for Build 2834134187

  • 140 of 162 (86.42%) changed or added relevant lines in 36 files are covered.
  • 12 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.0003%) to 84.042%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/instructionset.py 2 3 66.67%
qiskit/circuit/quantumcircuit.py 28 29 96.55%
qiskit/circuit/quantumcircuitdata.py 2 3 66.67%
qiskit/transpiler/passes/optimization/consolidate_blocks.py 1 2 50.0%
qiskit/transpiler/passes/optimization/template_matching/backward_match.py 0 1 0.0%
qiskit/transpiler/passes/optimization/template_matching/forward_match.py 0 1 0.0%
qiskit/dagcircuit/dagdepnode.py 0 2 0.0%
qiskit/quantum_info/operators/operator.py 4 6 66.67%
qiskit/visualization/dag_visualization.py 0 2 0.0%
qiskit/dagcircuit/dagcircuit.py 24 29 82.76%
Files with Coverage Reduction New Missed Lines %
qiskit/circuit/instruction.py 1 95.95%
qiskit/circuit/library/data_preparation/state_preparation.py 1 95.72%
qiskit/circuit/library/generalized_gates/linear_function.py 1 98.36%
qiskit/extensions/quantum_initializer/diagonal.py 1 89.86%
qiskit/extensions/quantum_initializer/squ.py 1 79.78%
qiskit/extensions/quantum_initializer/uc_pauli_rot.py 1 93.9%
qiskit/extensions/quantum_initializer/uc.py 1 88.11%
qiskit/quantum_info/operators/predicates.py 1 59.78%
qiskit/quantum_info/synthesis/one_qubit_decompose.py 1 97.21%
qiskit/transpiler/passes/optimization/template_matching/backward_match.py 1 79.72%
Totals Coverage Status
Change from base Build 2833682794: -0.0003%
Covered Lines: 56309
Relevant Lines: 67001

💛 - Coveralls

@alexanderivrii
Copy link
Contributor Author

A few questions/comments came up in the discussion with @eliarbel and @ShellyGarion:

  1. This PR undoes Temporarily remove Operation class usage for 0.20 #7840, restoring Operation abstract base class #7087, and unfortunately leading to a slight temporary slowdown as per Performance regression caused by #7087 #7528. However, I don't think that the slowdown is significant.

  2. One change, compared to Operation abstract base class #7087, is that Instruction now inherits from Operation, this makes the code a bit simpler, in that we need to check in several places isinstance(instruction, Operation) instead of isinstance(instruction, [Instruction, Operation]). I don't have a strong opinion on which alternative is better and open to suggestions.

  3. The test test_native_operations.py needs to be reorganized (and possibly renamed). The files optimize_cliffords.py and test_clifford_passes.py are currently more for demonstration purposes (what could we do if Cliffords were added as Operations), we may or may not include these in this PR.

@alexanderivrii alexanderivrii changed the title [WIP] Adding Cliffords to QuantumCircuits as Operations Adding Cliffords to QuantumCircuits as Operations Apr 27, 2022
Comment on lines 80 to 84
@property
@abstractmethod
def definition(self):
"""Definition of the operation in terms of more basic gates."""
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to carry this over to Operation since these are objects that we want to allow to not specify a decomposition at construction time (and especially, to not have to carry around a definition).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently multiple transpiler passes (Decompose, Unroll3qOrMore, etc.) require node.op.definition, so this is a method that had to be implemented for Cliffords and that imho Operation should support. However, the main concern is already addressed (for Cliffords): the definition circuit is not constructed at the gate creation time (and only when it's actually needed). For instance, the following code:

qc1 = QuantumCircuit(5)
qc1.append(random_clifford(3), [4, 0, 2])
qc1.append(random_clifford(3), [4, 0, 2])
qc1.append(random_clifford(3), [4, 0, 2])
qc2 = PassManager(OptimizeCliffords()).run(qc1)
qc3 = qc2.decompose()

only constructs the definition circuit for a single clifford in qc2 and only when decompose is called.

@@ -75,7 +75,7 @@ def __init__(self, data, input_dims=None, output_dims=None):
if isinstance(data, (list, np.ndarray)):
# Default initialization from list or numpy array matrix
self._data = np.asarray(data, dtype=complex)
elif isinstance(data, (QuantumCircuit, Instruction)):
elif isinstance(data, (QuantumCircuit, Operation)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other places where Instruction should be updated to Operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think I have updated all the places that I could find, but maybe I've still missed a few.

@@ -1160,7 +1161,7 @@ def _resolve_classical_resource(self, specifier):

def append(
self,
instruction: Instruction,
instruction: Operation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update the docstring below, and potentially rename the argument from instruction to operation. (There's a util function https://github.com/Qiskit/qiskit-terra/blob/04807a13a7ba53d97c37bb092324e419296e3fba/qiskit/utils/deprecation.py#L19 to simplify the process of deprecating the original kwarg.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the docstring. Would it be possible to postpone renaming/deprecation to a small follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't issue a deprecation warning until the alternative has been in place for a version. I wouldn't worry about it, though. Perhaps a long-term solution is (once we drop Python 3.7 support) to make this first argument positional-only (def append(self, instruction, /, qargs, cargs)), since append can also take a single CircuitInstruction as of #8093, but really, it's a minor problem we don't need to touch right now.

@@ -1261,7 +1262,11 @@ def _append(

return instruction

def _update_parameter_table(self, instruction: Instruction) -> Instruction:
def _update_parameter_table(self, instruction: Operation) -> Operation:
# A generic Operation object at the moment does not require to have params.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is the right time to reconsider whether Operations should require params (I don't recall the exact reason they were removed from the PR implementing Operation). If not, checking isinstance(instruction, Instruction) seems more consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code to check isinstance(instruction, Instruction).

I don't remember the reason of not including params in Operation either. Possibly this would be a good idea, even though Cliffords don't use params to store stabilizer/destabilizer table. Do we want them to? Is it true that two Instructions with the same name, num_qubits, num_clbits and parameters are completely equivalent? Do we want this to be true?

Comment on lines 571 to 577
@property
def definition(self):
"""Computes and returns the circuit for this Clifford."""
if self._definition is None:
self._definition = self.to_circuit()

return self._definition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, how far do we make it through a transpile call if we don't add a definition? The long term goal is to have a transpiler pass which can synthesize Cliffords and use the transpilation context to make some intelligent choices between different synthesis methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, can to_instruction above now return self instead of converting to a circuit and then to an instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how far do we make it through a transpile call if we don't add a definition?

I would say quite far, please see the code snippet from my answer to a previous question.

The long term goal is to have a transpiler pass which can synthesize Cliffords and use the transpilation context to make some intelligent choices between different synthesis methods.

I completely agree. This is also something that @ShellyGarion is interested in, as she wants to be able to choose at transpile time whether to apply a synthesis method that minimizes gate-count vs. depth and whether to apply a method that is better suited for all-to-all connectivity vs. linear-neighbor connectivity.

can to_instruction above now return self instead of converting to a circuit and then to an instruction?

This is an interesting suggestion (that I have not tried yet). I might be wrong about this, but I am afraid that certain methods (like constructing a unitary Operator for a quantum circuit containing a clifford) depend on to_operation translating more complex objects to simpler objects.

Comment on lines 61 to 63
blocks.append(cur_block)
prev_node = None
cur_block = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but this could be wrapped in an if cur_block: to avoid adding an empty [] to blocks for every non-Clifford instruction in the circuit.

Comment on lines +21 to +22
This serves as an example of extra capabilities enabled by storing
Cliffords natively on the circuit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think we should look to generalize CollectMultiQBlocks or similar so that we can efficiently collect and grow blocks to resynthesize, but that's a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should look to generalize CollectMultiQBlocks or similar so that we can efficiently collect and grow blocks to resynthesize, but that's a separate issue.

Agreed. This seems like some important functionality that comes up over and over.

blocks.append(cur_block)
cur_block = [node]
else:
if prev_node.qargs == node.qargs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if prev_node.qargs == node.qargs:
if set(prev_node.qargs) == set(node.qargs):

Can we match here even if the qarg orders don't match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking about this, and this should be possible, but we need to suitably permute the rows and the columns of the clifford's stabilizer tableau.

@alexanderivrii alexanderivrii requested a review from kdk May 10, 2022 13:23
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the path to optimize this Clifford is very important. However, I am against extending the Clifford in quantum_info to Operation/Instruction, etc. quantum_info and Circuit are on different levels of hierarchy in Qiskit, and confusion would create a circular dependency. For example, Operator is a class that represents operators, and unitary operators are represented using this class. But, this class is not Instruction. For unitary gates, another class UnitaryGate is used for QuantumCircuit. Therefore, it is safer to create the separate class such as CliffordGate or CliffordOperation to separate the hierarchy.

Another suggestion: how about finding clifford in Gates and compose it? (This could easily be done by putting an is_clifford flag into the Gate.) Wouldn't it be complicated to have to wrap in Clifford to represent Hadamard, S-Gate, etc.?

@alexanderivrii
Copy link
Contributor Author

@ikkoham, thank you for your feedback! This PR is the result of a discussion that has been going on for quite some time, please see #5811 (and #7087), and in particular @kdk's comment #5811 (comment).

You can view this as moving in the direction of high-level-synthesis, @kdk and @jakelishman can explain this better than me.

Please note that we can already add Cliffords onto a QuantumCircuit as per:

qc = QuantumCircuit(4)
cliff = random_clifford(2)
qc.append(cliff, [1, 3])

so to an extent the confusion already exists.

From the transpilation point of view, the above currently implicitly converts cliff to a Gate (and loses the Clifford structure of the object). This PR somewhat resolves this. I agree that this is not the only way to do this, and we could have (if I understand your suggestion correctly) define a class such as

CliffordGate(Gate):
  def __init__(self, cliff):
    # stores Clifford object
    self._cliff = cliff

and then call this like

qc.append(CliffordGate(cliff), [1, 3])

but frankly I don't see much benefit (and would require us to do many changes throughout the code).


We have discussed the current status of this PR during the last Terra Circuits meeting, and we have agreed that we do not want to include definition inside Operation (and we are currently discussing on how to resolve this issue).


@ikkoham, to see if I understand your second point. You are suggesting a transpiler pass that would collect blocks of consecutive Clifford gates, and merge these into a single Clifford object, is this correct? I.e. similar to recent work on defining LinearFunctions (https://qiskit.org/documentation/stubs/qiskit.circuit.library.LinearFunction.html) and combining these as per
https://qiskit.org/documentation/stable/0.35/stubs/qiskit.transpiler.passes.CollectLinearFunctions.html.

@ikkoham
Copy link
Contributor

ikkoham commented May 19, 2022

@alexanderivrii Thank you for your reply.

I have no objection to the hierarchy of quantum circuits, Operation and Instruction.
What I am commenting on is the relationship between qiskit.circuit and qiskit.quantum_info.

Please note that we can already add Cliffords onto a QuantumCircuit as per:'
...
so to an extent the confusion already exists.

Your example is just using implicit conversions. Without implicit conversion, we have

qc = QuantumCircuit(4)
cliff = random_clifford(2)
qc.append(cliff.to_instruction(), [1, 3])

and Clifford itself is NOT Instruction now.

If Clifford is made more complicated, it will be impossible to maintain. Personally, I believe that better code is one in which each module is loosely coupled. This direction of tight coupling makes maintenance difficult; the introduction of CliffordGate helps to weaken the coupling between modules.

(But, in past discussions, I have seen that UnitaryGate will be deprecated and replaced with Operator in quantum_info. If this is the direction to go, the contradiction I pointed out will be resolved. But personally, I think UnitaryGate is clearer and I don't want it to be deprecated... What is current direction?)

And, for qc.append(cliff, [1, 3]), the simplicity of the interface can be assured by applying the following conversion inside the append method.

if isinstance(instruction, Clifford):
    instruction = CliffordGate(inst)

This method allows qc.append(Clifford) directly.


to see if I understand your second point. You are suggesting a transpiler pass that would collect blocks of consecutive Clifford gates, and merge these into a single Clifford object, is this correct?

Yes. That is what I want to say. Thanks.

@jakelishman
Copy link
Member

@ikkoham: we totally agree with you - we're very much not trying to add any meaningful extra code to Clifford, and Clifford shouldn't be dependent on QuantumCircuit at all.

The below is a very rough sketch of one possible idea, but it's still under discussion and not decided at all:

We will define an Operation interface, which supplies only very basic information about the operation such as its number of qubits, classical bits and the types of any runtime parameters it needs. It doesn't specify a definition for itself in terms of a circuit. This Operation interface can be defined at a very low level - quantum_info will be able to import it without importing QuantumCircuit, so we maintain the right hierarchy.

QuantumCircuit will be able to contain things that implement Operation, but it will be the responsibility of the transpiler to convert these into gate-level representations. It won't be the individual classes that are responsible for this, but a consumer of them at the transpiler level. This "higher-order" synthesis may also consider multiple Clifford instances at once, and may have access to layout/routing/backend information at the time it does its synthesis. This is separating "data" (Clifford) from "behaviour" (various different synthesis algorithms), so we can build up everything in an extensible composable way, without needing one giant monolithic Clifford class (which is unmaintainable, as you say).

HighLevelSynthesis transpiler pass, and incorporating it into the preset
pass managers
…oadcast from Operation interface and in particular from Clifford
@alexanderivrii
Copy link
Contributor Author

Please refer to @jakelishman's proposal for Operation interface here.

Based on the recent discussions, I have removed both definition and broadcast from Operation's (and in particular Clifford's) API.

The code in this PR is significantly closer to the above proposal.


Regarding definition, we have a transpiler pass (tentatively called HighLevelSynthesis) that traverses the DAG and replaces each high-level-operation (only Cliffords for now, but in the future it will also contain LinearFunctions, CNOTDihedrals and other objects that will be native Operations) by standard gates using the appropriate synthesis algorithm.

In the future, we probably want each synthesis algorithm to be available via a plugin interface, similarly to the unitary synthesis plugin interface, thus HighLevelSynthesis may need to be later modified to accept a CliffordSynthesisPlugin, a LinearFunctionPlugin, etc., and possibly even the unitary synthesis plugin should also be a part of this. This is open for discussion.

As of now, I have modified the 4 preset passmanagers to run HighLevelSynthesis right after UnitarySynthesis but before Unroll3qOrMore, so that the Unroll3qOrMore pass can safely assume that every gate has a definition and use it for unrolling.

There is also a question whether the Decompose pass should decompose high-level-objects (e.g. native Cliffords) or leave them as they are, personally I believe that it should, so I have added HighLevelSynthesis to run as the first stage of Decompose. Comments are welcome.


Regarding broadcast. I have incorporated the ideas discussed in #7718 (and closed the latter PR). I have moved all of the broadcasting functionality to a separate broadcast.py file (I have not done any significant code changes, modulo replacing return by yield when appropriate), and have changed QuantumCircuit and QuantumCircuitData to call broadcast_arguments(instruction, qargs, cargs) instead of instruction.broadcast_arguments(qargs, cargs). I have removed the functionality broadcast_arguments from all of the relevant places (barrier, measure, etc.) -- since they were only used by QuantumCircuit.append, I believe that no deprecation mechanism is necessary here. There seems to be a significant opportunity to further unify and clean up the code in broadcast.py. I have added test_broadcast.py that checks the functionality of all different previously supported broadcasting schemes.

…pose; slightly changing Decompose pass to skip nodes with definition attribute
@alexanderivrii
Copy link
Contributor Author

@mtreinish, thank you for the review. I have improved/expanded the description in the release notes, and have added a TODO comment in HighLevelSynthesis. I agree that it would be good to expand the pass to be pluggable and to do this as soon as possible, especially considering the next point.

Even though the current code works correctly for Cliffords, as you have pointed out, there is a problem in Operator that will manifest for higher-level objects in the future. Basically, we need to know how to construct a matrix for any given higher-level object, so it seems that Operator has to be aware of HighLevelSynthesis. So it seems that we already need a generic HighLevelSynthesis plugin in order to handle Operator correctly. Given the size of this PR, the question is whether we really should have this plugin in this PR, or it's ok to have it in a follow-up PR, with the understanding that Operator code will not work with non-Clifford higher-level objects?

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the updates. This mostly LGTM for now. The only thing I'd like to see resolved is the use of Operation.to_matirx in Operator: https://github.com/Qiskit/qiskit-terra/pull/7966/files#r941549919 I think as long as we outline that to_matrix() is an optional thing on operations and that the code handles the path when we have an opaque instruction without a matrix representation that is sufficient to resolve this. Once that is fixed I think I'm fine approving this.

@alexanderivrii
Copy link
Contributor Author

alexanderivrii commented Aug 10, 2022

Thanks for making the updates. This mostly LGTM for now. The only thing I'd like to see resolved is the use of Operation.to_matirx in Operator: https://github.com/Qiskit/qiskit-terra/pull/7966/files#r941549919 I think as long as we outline that to_matrix() is an optional thing on operations and that the code handles the path when we have an opaque instruction without a matrix representation that is sufficient to resolve this. Once that is fixed I think I'm fine approving this.

@mtreinish, is the code comment in Operator (see 07733d6) good enough, or would you prefer to add a comment to Operation docs as well? I am somehow a bit reluctant to advertise the possibility of to_matrix() method in Operation (as this goes against the idea of this API).

Another thought is that if someone really wants to construct an Operator for a quantum circuit qc with higher-level objects, then he/she can just call qc.decompose() the right number of times, so it's not really a must-have functionality.

@mtreinish
Copy link
Member

I was thinking of having it as part of the documentation but I guess a comment is fine too if we don't want to advertise it. But I think you're right maybe we should just drop this piece as it really isn't a logical part of the operation class and revert back to using Instruction in the Operator class. The only concern there is from a backwards compatibility PoV if someone did:

qc.append(clifford)
Operator(qc)

that would break when it previously worked right? Maybe we just should special case things and do an isinstance for an Instruction or a clifford explicitly and leave a comment saying it's for backwards compatibility.

@alexanderivrii
Copy link
Contributor Author

qc.append(clifford)
Operator(qc)

that would break when it previously worked right?

Yes, this used to work, and some people (like @ShellyGarion) depend on this functionality. So we should support this for backward compatibility.

Maybe we just should special case things and do an isinstance for an Instruction or a clifford explicitly and leave a comment > saying it's for backwards compatibility.

This is a perfect suggestion, done in ab213f2. On the one hand, we don't include things to Operation's API, on the other hand there is no under-the-table non-documented functionality. And we can always revisit things in the future as more higher-level-operations will become available.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM now, thanks for sticking with this and making all the necessary updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants